Fix Pkg.BinaryPlatforms invalidations by moving module to Base #52249

mkitti · 2023-11-21T13:30:13Z

Move Pkg.BinaryPlatforms compatibility shim to Base.BinaryPlatforms.PkgCompat
~~Move UnstableIO from Pkg.jl to Base~~

Address Pkg.jl invalidations by moving code to Base
JuliaLang/Pkg.jl#3702

Edits

The UnstableIO portion was removed in favor of using IOContext{IO} instead: Replace UnstableIO with IOContext{IO} Pkg.jl#3735
Background reading for how Base.BinaryPlatforms came to Base and its relationship with Pkg.BinaryPlatforms: Move Artifacts and BinaryPlatforms into Base #37320

…kgCompat

Keno · 2023-11-21T21:13:18Z

base/io.jl

+
+Type unstable wrapper around an IO to avoid specializing write, get, and print
+"""
+struct UnstableIO <: IO


This is probably beside the point, but this is insufficient to avoid inference inferring these functions on specialized IO. For that you'd need to add an extra inferencebarrier in the UnstableIO constructor.

It worked pretty well in practice. :p

@Keno, would that look like the following?

struct UnstableIO <: IO io::IO UnstableIO(@nospecialize(x)) = new(inferencebarrier(x)) end

@Keno I added an inferencebarrier in 1f0876f

This struct also doesn't seem compliant with the IO interface, so I am very reluctant to have this merged into Base. Perhaps make this <: AbstractPipe might suffice though, as then you get all of the required methods "for free"

There's an IO interface? Let me see if I can find it. I'm going to see if I can figure this out quickly. Otherwise, I will separate this into another pull request.

staticfloat

LGTM

ararslan

Were there no tests in Pkg for these that could be copied over as well?

base/binaryplatforms_pkgcompat.jl

mkitti · 2023-12-07T19:29:40Z

I can port over https://github.com/JuliaLang/Pkg.jl/blob/master/test/binaryplatforms.jl

mkitti · 2023-12-08T00:11:01Z

I ported over the BinaryPlatforms tests in
9843555 .

I added Base.UnstableIO tests in 172909b

mkitti · 2023-12-09T06:21:14Z

@vtjnash
I refactored UnstableIO onto AbstractPipe in b55dc3d

mkitti · 2023-12-13T07:36:32Z

Is there anything else to do here? Should I split the PR into two parts?

vtjnash · 2023-12-13T13:09:05Z

base/io.jl

@@ -1520,3 +1520,17 @@ function countlines(io::IO; eol::AbstractChar='\n')
 end

 countlines(f::AbstractString; eol::AbstractChar = '\n') = open(io->countlines(io, eol = eol), f)::Int
+
+# From Pkg.jl


Would it be easier to make Pkg instead use an IOContext{IO}(io), since it has the same implementation as this, without needing to add code to Base just for Pkg?

I was thinking about that, but IOContext has a type parameter indicating the parent type. I thought the objective was to create an abstract inference barrier so as to not force recompilation of Pkg methods for new IO types.

I'm thinking of splitting the pull request so that we can focus on this issue.

That is true of the usual interface, but it is valid for the parameter to be simply IO also. Someone could make a PR to make that constructor a bit simpler also, as right now it is potentially a bit awkwardly specified as IOContext{IO}(unwrapcontext(io)...) and does not reliably propagate when making a new IOContext from an existing one with more keys.

I see so I may be able to define the following.

UnstableIO(io:IO) = IOContext{IO}(inferencebarrier(io))

mkitti · 2023-12-13T16:36:26Z

Unless I hear otherwise, I intend to drop the UnstableIO part of the PR from this pull request.

vtjnash · 2023-12-13T16:59:31Z

Why do we need all the other stuff here too? It seems like if Pkg wants to export something with a similar name that does something different it should export something different, rather than pirating the method from Base or expecting Base to contain that functionality.

mkitti · 2023-12-13T17:31:58Z

Right now I'm looking for pragmatic steps to take to improve load times of Pkg as an independent package. My overarching motivation is to reduce the negative impact on the user experience that breaking these packages out creates.

To be fair, it was Base that committed the piracy against Pkg. This was originally a Pkg API.

The code here is a compatability shim because the API changed when this functionality moved from Pkg to Base. Now that Pkg.jl is no longer part of the system image, we have invalidation issues due that prior solution. While one could debate this, we are obligated to carry that compatibility shim, I believe.

I'm going to invite @staticfloat and @giordano to comment on where the older BinaryPlatforms API is still used and why we need the compatibility shim.

mkitti · 2023-12-18T02:38:46Z

I proposed an alternative approach is JuliaLang/Pkg.jl#3736 . That change is breaking in that it eliminates the ability to dispatch on the Linux, Windows, and MacOS types. Those are now constructors for Base.BinaryPlatforms.Platform. Surveying JuliaHub for use of the symbols, only a single registered package, AppBundler.jl, was found to be affected by the breakage so far.

ararslan · 2023-12-21T18:18:11Z

FWIW the point of those types was to enable dispatch, which I've personally used extensively, just not in any registered packages.

mkitti · 2023-12-22T08:35:54Z

I have proposed another alternative in JuliaLang/Pkg.jl#3742 , this time keeping the types. To avoid invalidations, I do not subtype Base.BinaryPlatforms.AbstractPlatform.

mkitti added 2 commits November 21, 2023 08:25

Move Pkg.BinaryPlatforms compatibility shim to Base.BinaryPlatforms.P…

bc35288

…kgCompat

Move UnstableIO from Pkg.jl to Base

912a020

mkitti mentioned this pull request Nov 21, 2023

snoopi_deep results, stale instances JuliaLang/Pkg.jl#3703

Open

ViralBShah added the domain:packages Package management and loading label Nov 21, 2023

Keno reviewed Nov 21, 2023

View reviewed changes

mkitti mentioned this pull request Nov 28, 2023

Fix Pkg.jl invalidations JuliaLang/Pkg.jl#3712

Draft

Add an inferencebarrier to the UnstableIO constructor

1f0876f

staticfloat approved these changes Nov 29, 2023

View reviewed changes

ararslan reviewed Dec 7, 2023

View reviewed changes

base/binaryplatforms_pkgcompat.jl Outdated Show resolved Hide resolved

mkitti added 5 commits December 7, 2023 15:51

Add binary platform tests

9843555

Fix spelling of fuchsia_libc

56cfd08

Change test module name and add a comment about origin of the tests

3d460fa

Remove indenting from comment

bca6b82

Add tests for Base.UnstableIO

172909b

Refactor UnstableIO to be an AbstractPipe

b55dc3d

vtjnash reviewed Dec 13, 2023

View reviewed changes

Remove UnstableIO

8c36b11

This was referenced Dec 17, 2023

Replace UnstableIO with IOContext{IO} JuliaLang/Pkg.jl#3735

Merged

Refactor Pkg.BinaryPlatforms compat, fix invalidations JuliaLang/Pkg.jl#3736

Closed

mkitti changed the title ~~Fix Pkg.jl invalidations by moving code to Base~~ Fix Pkg.BinaryPlatforms invalidations by moving module to Base Dec 18, 2023

mkitti mentioned this pull request Dec 22, 2023

Refactor Pkg.BinaryPlatforms to avoid invalidations, keep types JuliaLang/Pkg.jl#3742

Open

mkitti closed this Jan 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Pkg.BinaryPlatforms invalidations by moving module to Base #52249

Fix Pkg.BinaryPlatforms invalidations by moving module to Base #52249

mkitti commented Nov 21, 2023 •

edited

Keno Nov 21, 2023

KristofferC Nov 21, 2023

mkitti Nov 21, 2023

mkitti Nov 28, 2023

vtjnash Dec 8, 2023

mkitti Dec 8, 2023

staticfloat left a comment

ararslan left a comment

mkitti commented Dec 7, 2023

mkitti commented Dec 8, 2023

mkitti commented Dec 9, 2023

mkitti commented Dec 13, 2023

vtjnash Dec 13, 2023

mkitti Dec 13, 2023

vtjnash Dec 13, 2023

mkitti Dec 13, 2023

mkitti commented Dec 13, 2023

vtjnash commented Dec 13, 2023

mkitti commented Dec 13, 2023 •

edited

mkitti commented Dec 18, 2023 •

edited

ararslan commented Dec 21, 2023

mkitti commented Dec 22, 2023

Fix Pkg.BinaryPlatforms invalidations by moving module to Base #52249

Fix Pkg.BinaryPlatforms invalidations by moving module to Base #52249

Conversation

mkitti commented Nov 21, 2023 • edited

Edits

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staticfloat left a comment

Choose a reason for hiding this comment

ararslan left a comment

Choose a reason for hiding this comment

mkitti commented Dec 7, 2023

mkitti commented Dec 8, 2023

mkitti commented Dec 9, 2023

mkitti commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented Dec 13, 2023

vtjnash commented Dec 13, 2023

mkitti commented Dec 13, 2023 • edited

mkitti commented Dec 18, 2023 • edited

ararslan commented Dec 21, 2023

mkitti commented Dec 22, 2023

mkitti commented Nov 21, 2023 •

edited

mkitti commented Dec 13, 2023 •

edited

mkitti commented Dec 18, 2023 •

edited